Skip to content

add shaded guava collections and use it #2086

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 22 commits into from
Nov 10, 2020
Merged

Conversation

andimarek
Copy link
Member

@andimarek andimarek commented Oct 29, 2020

This is an internal change only: we are using guava Immutable collections now in a lot places for more performance and more clarity.

return (ImmutableMapWithNullValues<K, V>) values;
}
return new ImmutableMapWithNullValues<>(values);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didnt want to put a of helper here

ImmutableMapWithNullValues.of("k1", "v1")
ImmutableMapWithNullValues.of("k1", "v1", "k2", "v2")

@@ -66,7 +69,7 @@ private DataFetchingEnvironmentImpl(Builder builder) {
this.locale = builder.locale;
this.operationDefinition = builder.operationDefinition;
this.document = builder.document;
this.variables = builder.variables == null ? Collections.emptyMap() : builder.variables;
this.variables = builder.variables == null ? ImmutableMapWithNullValues.empty() : builder.variables;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe renamed ImmutableMapWithNullValues.empty() =to ImmutableMapWithNullValues.emptyMap() - this would be clearer in my book and also better on static import

this.variables = builder.variables == null ? emptyMap() : builder.variables;

@dfa1
Copy link
Contributor

dfa1 commented Nov 3, 2020

Quite happy to see this PR! I believe that the approach is very good overall and ImmutableList is a very nice type to read. And it is quite nice to see no transitive dependency on Guava.

In JDK11 there are also immutable maps and lists with some nice goodies (e.g. smartcopyOf, no nulls, safe serialization). What are your plans regarding JDK11+?

Besides that, I would like to test this PR in our project and report some early feedback.

@bbakerman
Copy link
Member

What are your plans regarding JDK11+?

graphql-java plans to be a late adopter of JDK 11 - namely because so many users are not there yet.

return instrumentation.beginField(parameters.withNewState(state));
})
.collect(toList()));
return new ChainedInstrumentationContext<>(map(instrumentations, instrumentation -> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the format change here?

Copy link
Member Author

@andimarek andimarek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@@ -169,7 +196,7 @@ public GraphQLObjectType getObjectType(String typeName) {
}

public Map<String, GraphQLNamedType> getTypeMap() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't nee to be a copy

@andimarek andimarek changed the title add shaded guava collections and use it in Field add shaded guava collections and use it Nov 10, 2020
@andimarek andimarek added this to the 16.0 milestone Nov 10, 2020
@bbakerman bbakerman merged commit f6f9bae into master Nov 10, 2020
@andimarek andimarek deleted the immutable-collections-guava branch May 4, 2021 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants